fix(cdrmodal): fixes the issue where screen readers can read content … #151
Conversation
…outside the modal Upon mounting, any modal instances will now be teleported to the root of the document.body element. When the modal is opened, other top-level children of the body element will be marked aria-hidden to prevent them from being read by certain screen readers. BREAKING CHANGE: This may not be a breaking change for teams, but they will need to test that it works for their application. Because this will move the modal from the spot they're originally mounting it to the body element, there could be unexpected effects if they were relying on it to remain in that spot.
| <!-- start: testing modal content show/hide --> | ||
| <div | ||
| aria-hidden="true" | ||
| style="opacity: 0;" | ||
| >something already aria-hidden</div> | ||
| <div hidden>something display: none;</div> | ||
| <div style="opacity: 0;">another div of outside content, not aria-hidden</div> | ||
| <!-- end: testing modal content show/hide --> |
There was a problem hiding this comment.
TODO this will be removed, for display/testing only
src/components/modal/CdrModal.vue
Outdated
| @@ -92,11 +102,11 @@ let lastActive: Element | null; | |||
| const modalClosed = ref(!props.opened); | |||
| const isOpening = ref(false); | |||
|
|
|||
| interface offsetValues { | |||
| interface OffsetValues { | |||
There was a problem hiding this comment.
Sonarlint caught this formatting.
There was a problem hiding this comment.
I'd ask to remove these changes, we use a different config
| @@ -190,6 +201,62 @@ const removeNoScroll = () => { | |||
| } | |||
| }; | |||
|
|
|||
There was a problem hiding this comment.
Modal scripts begin below
| ':not(style)', | ||
| ':not([data-is-modal])', | ||
| ':not([aria-hidden=true])', | ||
| ] |
There was a problem hiding this comment.
Summary of these selectors: find all direct children of the body element, that are not:
- script or style tags
- that are not this modal or other modals (they should be hidden when the top modal is open anyway)
- that are not already
aria-hidden
|
|
||
| // for the remaining, check if they should be hidden | ||
| contentBodyChildren.forEach((el) => { | ||
| if (getComputedStyle(el).getPropertyValue('display') !== 'none') { |
There was a problem hiding this comment.
Also, skip any that have a computed value of display: none.
| contentBodyChildren.forEach((el) => { | ||
| if (getComputedStyle(el).getPropertyValue('display') !== 'none') { | ||
| // TODO - temp to be sure we tagged right | ||
| el.setAttribute('data-modal-hide', ''); |
There was a problem hiding this comment.
This isn't strictly necessary, but would be pretty helpful in debugging. I'm 50/50 on whether to leave it or remove it.
| const hideModalBackgroundContent = () => { | ||
| // only run the expensive DOM scraping once | ||
| if (!backgroundNodesDiscovered) { | ||
| findBackgroundContentNodes(); |
There was a problem hiding this comment.
Put this here instead of in a beforeMount hook, for example.
Two reasons:
- Performance: this may never need to run at all.
- Our pages continue loading content at the body level way past when the
DOMContentLoadedevent fires. Playing around with this in the PDP, I had to use a setTimeout of something like 5 seconds (on a fast connection) to get all of our littlebodychildren loaded.
| const textContentStyle = computed(() => ({ | ||
| maxHeight: `${scrollMaxHeight.value}px`, | ||
| paddingRight: `${scrollPadding.value}px`, | ||
| })); | ||
|
|
There was a problem hiding this comment.
Moved this out of template - was throwing a lint error for line length having it inline below.
| :class="mapClasses(style, baseClass, !opened ? 'cdr-modal--closed' : '')" | ||
| ref="wrapperEl" | ||
| > | ||
| <Teleport to="body"> |
There was a problem hiding this comment.
template change 1
| :class="[style['cdr-modal__outerWrap'], wrapperClass]" | ||
| :class="mapClasses(style, baseClass, !opened ? 'cdr-modal--closed' : '')" | ||
| ref="wrapperEl" | ||
| data-is-modal |
There was a problem hiding this comment.
template change 2
| @@ -295,92 +372,91 @@ onUnmounted(() => { | |||
| window.removeEventListener('resize', handleResize); | |||
| }); | |||
|
|
|||
|
|
|||
| </script> | |||
|
|
|||
| <template> | |||
There was a problem hiding this comment.
lots of formatting fixes in the modal, noted actual changes below.
| // TODO not sure why this was setup like this? | ||
| opened: false, | ||
| // opened: this.$route.name === 'Modals', |
There was a problem hiding this comment.
Side fix: I found this really difficult to develop with - I needed to see the page state before and after modal load. I wasn't sure if there was a reason we had it load on going to the route, but if not I'd love to leave it like this.
No description provided.